Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Winch aarch64 dynamic heap #9252

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

vulc41n
Copy link
Contributor

@vulc41n vulc41n commented Sep 16, 2024

Hey 👋

This PR implements everything needed to run winch for aarch64 with dynamic heap. It implements the following macro instructions : trap, trapif, cmov and checked_add.

@vulc41n vulc41n requested review from a team as code owners September 16, 2024 09:20
@vulc41n vulc41n requested review from elliottt and removed request for a team September 16, 2024 09:20
@github-actions github-actions bot added the winch Winch issues or pull requests label Sep 16, 2024
Copy link

Subscribe to Label Action

cc @saulecabrera

This issue or pull request has been labeled: "winch"

Thus the following users have been cc'd because of the following labels:

  • saulecabrera: winch

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Left some comments which we should address/clarify.

Comment on lines 238 to 240
fn cmov(&mut self, src: Reg, dst: Reg, cc: IntCmpKind, size: OperandSize) {
self.asm.jmp_if_resolved(Cond::from(cc).invert(), 12);
self.asm.mov_rr(src, dst, size)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better to handle this instruction via csel. Any particular reason you decided to go this route?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I didn't think about it

todo!()
fn checked_uadd(&mut self, dst: Reg, lhs: Reg, rhs: RegImm, size: OperandSize, trap: TrapCode) {
self.add(dst, lhs, rhs, size);
self.asm.trapif(Cond::Vs, trap);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe here we'd want to use Cond::Hs? The Cond::Vs is for signed overflow. (Ref: https://developer.arm.com/documentation/dui0801/l/Condition-Codes/Carry-flag)

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@saulecabrera saulecabrera added this pull request to the merge queue Sep 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 20, 2024
@saulecabrera
Copy link
Member

@vulc41n it looks like an update to the filetests is needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants